feat(auth): add failproofai auth login/logout/whoami + fix 3 over-firing builtin policies#396
Open
NiveditJain wants to merge 7 commits into
Open
feat(auth): add failproofai auth login/logout/whoami + fix 3 over-firing builtin policies#396NiveditJain wants to merge 7 commits into
NiveditJain wants to merge 7 commits into
Conversation
…itive fixes Auth CLI (the follow-up promised by 0.0.11-beta.2's removal of the old device-flow auth in #380): - `failproofai auth login [--email <addr>]` — POSTs to /v0/auth/login/request, prompts for the 6-digit OTP, exchanges it via /v0/auth/login/verify, persists the session. - `failproofai auth logout` — best-effort POST /v0/auth/logout, then always clears local state so the user can never get stuck with a session file they cannot rm. - `failproofai auth whoami` — GET /v0/auth/me with the access token, auto-refreshing via /v0/auth/token/refresh when the access token is expired but the refresh token is still valid. New `src/auth/{session-store,api-client,prompts,manager}.ts` and matching tests. Session stored at `~/.failproofai/session.json` with mode 0600 (same pattern as `src/audit/cache.ts`). Default base URL `https://api.befailproof.ai`; override via `FAILPROOFAI_API_BASE_URL`. Wired into `bin/failproofai.mjs` alongside `policies` and `audit`. While here, also fix a packaging quirk: bun's bundler can emit multiple copies of `CliError` when the same class shows up both via a static import inside a dynamically-imported handler and the top-level dynamic import in `bin/failproofai.mjs`. `err instanceof CliError` then returns false across the two copies and a clean user-facing error gets reported as "Unexpected error: ...". The top-level catch is now duck-typed (checks `err.name === "CliError"` + the `exitCode` field) so it works regardless of class identity in the bundle. Builtin-policy fixes (#NNN): - `block-secrets-write` no longer denies `Write` of any path containing the substring "credentials" or "id_rsa". `SECRET_FILE_CREDENTIALS_RE` now anchors to well-known credential paths (.aws/credentials, .docker/credentials.json, .netrc, …); `SECRET_FILE_ID_RSA_RE` now requires the SSH-key basename (so `id_rsa_backup.md` is allowed but `<anywhere>/id_rsa` is still blocked); `.pem/.key` extensions still block. Was over-firing on source files like `src/auth/credentials.ts`. - `sanitize-connection-strings` no longer flags grep/perl arguments that merely contain a scheme name. The regex now requires a URL-safe `<user>:<pass>@<host>` shape, so regex metachars in the pre-`@` segment (e.g. `postgres://|mysql://|@host:port/`) break the match. Real `postgresql://user:pw@host` strings still get redacted. - `warn-repeated-tool-calls` canonicalizes the fingerprint (sorts input keys recursively) so the same logical call always hashes the same regardless of property order, and records a `warned` set in the sidecar so the warning fires once per fingerprint per session instead of repeating every turn. Back-compat with the old bare-counts sidecar format is preserved. Tests: - New `__tests__/auth/` for session-store, api-client (mocked fetch), and manager (mocked fetch/prompts/session-store). - New cases under `__tests__/hooks/builtin-policies.test.ts` for the three policy false-positives — both that the previous misfires now return `allow` and that the genuine bad-input cases still `deny`. All 1719 unit tests pass, all 296 e2e tests pass, tsc/eslint clean. A clean `npm pack` + `npm install -g <tarball>` smoke verified the three commands and the duck-typed CliError catch end-to-end. Docker E2E against a real api-server container is in the plan but Docker Desktop's WSL integration was off in this environment, so the Docker portion of the plan is unverified here. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds email-OTP CLI auth (login/logout/whoami) with a typed auth HTTP client, interactive prompts, on-disk session persistence (0600), CLI entrypoint wiring, tests, and tightens builtin policy detection and repeated-tool-call fingerprinting. ChangesEmail-OTP Authentication Flow
Sequence Diagram (login, verify, persist): sequenceDiagram
participant User
participant CLI as "failproofai CLI"
participant ApiClient as "src/auth/api-client"
participant AuthServer
participant Store as "session store"
User->>CLI: run "failproofai auth login" (enter email/code)
CLI->>ApiClient: requestLoginCode(baseUrl, email)
ApiClient->>AuthServer: POST /v0/auth/login/request { email }
AuthServer-->>ApiClient: 200 LoginRequestResponse
CLI->>ApiClient: verifyLoginCode(baseUrl, email, code)
ApiClient->>AuthServer: POST /v0/auth/login/verify { email, code }
AuthServer-->>ApiClient: 200 TokenResponse
CLI->>Store: writeSession(AuthSession)
Store-->>CLI: persisted (~/.failproofai/session.json)
CLI-->>User: "Logged in."
Builtin Policy Precision Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
__tests__/auth/manager.test.ts (1)
194-208: ⚡ Quick winAdd a transient refresh-failure test case (should not clear session).
Please add a test where
refreshTokensfails with a retryable error (e.g., network/5xx) and assertclearSessionis not called. This protects the intended “clear only when session is expired” behavior.As per coding guidelines: “Always add unit tests for new behaviour. Place tests in tests/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/auth/manager.test.ts` around lines 194 - 208, Add a new unit test variant for whoamiCmd that sets up sessionMocks.readSession to return an expired access token but a refresh token that is not expired, then mock apiClientMocks.refreshTokens to reject with a transient error (e.g., network/5xx) and assert that whoamiCmd rejects with the refresh error and that sessionMocks.clearSession was NOT called; use the same test structure as the existing expired-refresh test but replace the refresh token expiry and have apiClientMocks.refreshTokens.mockRejectedValueOnce(...) and assert expect(sessionMocks.clearSession).not.toHaveBeenCalled() while still asserting refreshTokens was invoked once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: Two CHANGELOG.md entries still use the placeholder "(`#NNN`)"; replace
those placeholders with the actual PR numbers and ensure each entry remains a
single line with the description followed by the PR reference (e.g.,
"description (`#1234`)"); update both occurrences of "(`#NNN`)" in the CHANGELOG.md
so they reference the correct PRs and confirm formatting matches the project's
one-line-per-entry guideline.
In `@src/auth/api-client.ts`:
- Around line 80-85: The fetch calls in the API client helpers (the POST helper
shown and the other fetch helper later in the file) currently let network errors
and timeouts bubble up as native errors; wrap each fetch invocation (e.g.,
inside the functions that call fetch at the shown diff and the helper at lines
102-106) in a try/catch, and on any thrown error rethrow a CliError with a
concise user-facing message (e.g., "Network error contacting auth server" or
"Request to auth server timed out") that includes the original error message;
preserve the original error as the cause if CliError supports it to retain
diagnostics.
In `@src/auth/manager.ts`:
- Around line 94-97: The catch block in whoamiCmd (and surrounding refresh
logic) currently always calls clearSession() on any error; change it to only
clear the local session when the refresh failure is definitive (e.g.,
unauthorized/expired token responses such as HTTP 401 or provider error codes
like invalid_grant), otherwise do not clearSession() and rethrow a retryable
CliError. Concretely, inspect the caught err for API response metadata (e.g.,
err.response?.status or provider error code) and only invoke clearSession() when
it indicates token invalid/expired; for other errors preserve the session and
throw the original or a retryable error message instead.
In `@src/hooks/builtin-policies.ts`:
- Around line 966-985: The warning fires one call too late and reports the wrong
count: change the trigger to evaluate the current call (use prevCount + 1 >=
THRESHOLD) and update the message to report the current total (use prevCount +
1) instead of prevCount; keep the rest of the flow (incrementing
tracker.counts[fingerprint] and setting tracker.warned[fingerprint] when
shouldWarn) the same and reference the existing symbols: prevCount, THRESHOLD,
shouldWarn, tracker.counts[fingerprint], tracker.warned[fingerprint],
fingerprint, and ctx.toolName.
- Around line 183-186: The three regexes (SECRET_FILE_RE, SECRET_FILE_ID_RSA_RE,
SECRET_FILE_CREDENTIALS_RE) must be adjusted to (1) accept both POSIX and
Windows separators and (2) avoid matching public key files like id_rsa.pub.
Update SECRET_FILE_RE to account for backslashes (e.g. use [\/\\] or make the
pattern match a filename with a backslash option) so Windows paths are
recognized; change SECRET_FILE_ID_RSA_RE to use a separator class (?:^|[\/\\])
and add a negative lookahead to exclude names ending in .pub (e.g.
(?:^|[\/\\])id_(?:rsa|dsa|ecdsa|ed25519)(?!\.pub)$); and update
SECRET_FILE_CREDENTIALS_RE to use (?:^|[\/\\]) everywhere instead of (?:^|\/) so
.aws, .docker, .kube, .azure, .gcloud, .config/gcloud and .netrc are matched on
Windows paths as well. Ensure you keep the same logical groups/names
(SECRET_FILE_RE, SECRET_FILE_ID_RSA_RE, SECRET_FILE_CREDENTIALS_RE) when
replacing the patterns.
---
Nitpick comments:
In `@__tests__/auth/manager.test.ts`:
- Around line 194-208: Add a new unit test variant for whoamiCmd that sets up
sessionMocks.readSession to return an expired access token but a refresh token
that is not expired, then mock apiClientMocks.refreshTokens to reject with a
transient error (e.g., network/5xx) and assert that whoamiCmd rejects with the
refresh error and that sessionMocks.clearSession was NOT called; use the same
test structure as the existing expired-refresh test but replace the refresh
token expiry and have apiClientMocks.refreshTokens.mockRejectedValueOnce(...)
and assert expect(sessionMocks.clearSession).not.toHaveBeenCalled() while still
asserting refreshTokens was invoked once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd5cc18f-b344-44f9-8528-fd880ee849ee
📒 Files selected for processing (11)
CHANGELOG.md__tests__/auth/api-client.test.ts__tests__/auth/manager.test.ts__tests__/auth/session-store.test.ts__tests__/hooks/builtin-policies.test.tsbin/failproofai.mjssrc/auth/api-client.tssrc/auth/manager.tssrc/auth/prompts.tssrc/auth/session-store.tssrc/hooks/builtin-policies.ts
- api-client: wrap fetch/timeout failures in CliError so network errors
land on the "Error:" exit-1 path instead of "Unexpected error". The
thrown error from HTTP responses carries a `.status` field so callers
can distinguish "transient" from "auth-rejected".
- manager.whoamiCmd: only clear local session on a refresh response with
status 401/403. Transient errors (network, 5xx, timeout) now preserve
the session and surface a "Could not refresh session" message so the
user can retry instead of being silently logged out.
- builtin-policies / block-secrets-write:
- `SECRET_FILE_ID_RSA_RE` no longer matches `id_rsa.pub` etc. — public
keys are not secrets. The trailing `(?:\\.pub)?` was removed and
`$` now anchors at the bare private-key basename.
- `SECRET_FILE_ID_RSA_RE` and `SECRET_FILE_CREDENTIALS_RE` accept both
POSIX `/` and Windows `\` separators so `C:\Users\me\.ssh\id_rsa`
and `C:\Users\me\.aws\credentials` are caught.
- Tests: add cases for `.pub` allowed, Windows secret paths denied,
network-error → CliError, timeout → CliError, 401 from refresh clears
session, transient refresh failure preserves session.
(Skipping the "warn on 3rd call not 4th" suggestion in
warn-repeated-tool-calls: the existing test fixture asserts the
warning fires with sidecar count=3 and the message string "3 times".
Both the threshold semantic and the message wording would have to
change in lockstep to flip to nextCount; deferred so this PR stays
scoped to the false-positive fixes the user reported.)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 13-18: The changelog contains multi-line bullets that must be
collapsed into single-line entries; find the multi-line entry beginning with
'access token, surfaces "Session expired"' (and the similar block later) in
CHANGELOG.md and rewrite each as a single-line bullet ending with the PR
reference (e.g., "(`#396`)"), preserving the original wording but joining lines
and removing extra line breaks so every changelog entry is one line as per
guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ets per coding guideline
Track and report the count *including* the current call (`nextCount = prevCount + 1`) and warn when `nextCount >= THRESHOLD`. With THRESHOLD = 3 that means the first warning fires on the 3rd identical call (previously it took 4) and the message correctly reports how many times the tool has been called including this one. Existing tests updated to seed sidecar count=2 so the 3rd call (the current one) hits the threshold. Addresses the last CodeRabbit comment on #396. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two-in-one PR.
failproofai auth login | logout | whoami(new)The follow-up promised by 0.0.11-beta.2's removal of the old device-flow auth (#380). Email-OTP login against the new Rust api-server (
/v0/auth/*):failproofai auth login [--email <addr>]— sends a one-time code viaPOST /v0/auth/login/request, prompts for the 6-digit code, exchanges it viaPOST /v0/auth/login/verify, persists the session.failproofai auth logout— best-effortPOST /v0/auth/logout, then always clears local state so the user can never get stuck with a session file they cannotrm.failproofai auth whoami—GET /v0/auth/mewith the access token, auto-refreshing viaPOST /v0/auth/token/refreshwhen the access token is expired but the refresh token is still valid; surfacesSession expiredand clears state when the refresh token has also expired.New
src/auth/{session-store,api-client,prompts,manager}.tsand matching__tests__/auth/. Session stored at~/.failproofai/session.jsonwith mode0600(same pattern assrc/audit/cache.ts). Default base URLhttps://api.befailproof.ai; override viaFAILPROOFAI_API_BASE_URL. Wired intobin/failproofai.mjsalongsidepoliciesandaudit, withtrack("cli_auth_<verb>_success")telemetry.While here, also fixes a packaging quirk: bun's bundler can emit multiple copies of
CliErrorwhen the class shows up via both a static import inside a dynamically-imported handler AND the top-level dynamic import inbin/failproofai.mjs.err instanceof CliErrorthen returns false across the copies and a clean user-facing error gets reported asUnexpected error: .... The top-level catch is now duck-typed (checkserr.name === "CliError"+ theexitCodefield) so it works regardless of class identity in the bundle.Three builtin policies stop over-firing on benign source / args
These were blocking ordinary development work — including, ironically, this PR.
block-secrets-writeno longer deniesWriteof any path containing the substringcredentialsorid_rsa.SECRET_FILE_CREDENTIALS_REnow anchors to well-known credential paths (.aws/credentials,.docker/credentials.json,.netrc, …);SECRET_FILE_ID_RSA_REnow requires the SSH-key basename soid_rsa_backup.mdis allowed but<anywhere>/id_rsais still blocked..pem/.keyextensions still block (plus.p12/.pfx/.jksadded for completeness). Was previously denying source files likesrc/auth/credentials.ts.sanitize-connection-stringsno longer flagsgrep/perlarguments that merely contain a scheme name. The regex now requires a URL-safe<user>:<pass>@<host>shape so regex metachars in the pre-@segment (e.g.'postgres://|mysql://|@host:port/'in a grep pattern) break the match. Realpostgresql://user:pw@hoststrings still get redacted.warn-repeated-tool-callscanonicalizes the fingerprint (sorts input keys recursively) so the same logical call always hashes the same regardless of property order, and records awarnedset in the sidecar so the warning fires once per fingerprint per session instead of repeating every turn. Back-compat with the old bare-counts sidecar format is preserved.Test plan
bun run test:run— 1719 passed (includes new__tests__/auth/*+ new false-positive cases under__tests__/hooks/builtin-policies.test.ts)bun run test:e2e— 296 passed (2 skipped)bunx tsc --noEmit— cleanbun run lint— clean (only pre-existing warnings)bun run build— Next + dist bundles producednpm pack --ignore-scripts→npm install --prefix <tmp>→ run the three subcommands against a clean install —auth --help,auth logout(no session),auth whoami(no session) all output correctly andauth whoamireturnsError: Not logged in.(verifying the duck-typedCliErrorcatch works across bundle boundaries).docker build -t failproof-api-server:e2e failproofai/api-server(in monorepo)postgres:16-alpine+ the api-server container withFAILPROOF_ENVIRONMENT=local,FAILPROOF_EMAIL_SENDER_BACKEND=logFAILPROOFAI_API_BASE_URL=http://failproof-api:8080 failproofai auth login --email <addr>→ grep OTP fromdocker logs failproof-api→ feed to stdin → assertauth whoamireturns the user → assertauth logoutclears~/.failproofai/session.json.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
failproofai auth login(email OTP interactive or --email),auth logout, andauth whoamiBug Fixes
Tests